crypto: fix handling of null BUF_MEM* in ToV8Value()#61885
Merged
nodejs-github-bot merged 4 commits intonodejs:mainfrom Feb 25, 2026
Merged
crypto: fix handling of null BUF_MEM* in ToV8Value()#61885nodejs-github-bot merged 4 commits intonodejs:mainfrom
nodejs-github-bot merged 4 commits intonodejs:mainfrom
Conversation
The assignment to `bptr` calls `BIO_get_mem_ptr` which can fail and leave the `bptr` as nullptr. This then later causes a null pointer deref. This is inconsistent with uses of the similar function `BIO_get_mem_data` that do check its return value, e.g. `node::crypto::X509sToArrayOfStrings()`. Solve it by checking for a null pointer and handling the `Nothing` return value at the call sites.
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61885 +/- ##
==========================================
+ Coverage 89.72% 89.77% +0.04%
==========================================
Files 675 674 -1
Lines 204806 205624 +818
Branches 39355 39425 +70
==========================================
+ Hits 183761 184596 +835
+ Misses 13330 13271 -59
- Partials 7715 7757 +42
🚀 New features to boost your workflow:
|
jasnell
approved these changes
Feb 20, 2026
addaleax
requested changes
Feb 20, 2026
Member
addaleax
left a comment
addaleax
left a comment
There was a problem hiding this comment.
The BUF_MEM* changes are fundamentally reasonable, although they are a strong argument why they really should not be using implicit casting operators. However, both looking at the OpenSSL source code and documentation, there seems to be no indication that these would be able to fail.
The ToV8Value() changes are definitely incorrect; if ToV8Value() does not already schedule an exception to be thrown when returning an empty value, that's a bug in ToV8Value() itself.
Contributor
Author
I suppose it depends. The OpenSSL documentation is incomplete as usual and doesn't indicate what the return value means. Looking at the source, a registered callback could still trigger a failure. LibreSSL puts the failure more explicitly: https://man.openbsd.org/BIO_s_mem.3 where it indicates "return 1 on success or a value less than or equal to 0 if an error occurred." |
Member
|
@ndossche Yeah, and like I said, it's a fundamentally reasonable change. My "request changes" bit primarily refers to the |
addaleax
approved these changes
Feb 25, 2026
addaleax
added
request-ci
github-actions
bot
removed
the
request-ci
Collaborator
addaleax
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
Collaborator
|
Landed in 488a854 |
aduh95
pushed a commit
that referenced
this pull request
Feb 28, 2026
The assignment to `bptr` calls `BIO_get_mem_ptr` which can fail and leave the `bptr` as nullptr. This then later causes a null pointer deref. This is inconsistent with uses of the similar function `BIO_get_mem_data` that do check its return value, e.g. `node::crypto::X509sToArrayOfStrings()`. Solve it by checking for a null pointer and handling the `Nothing` return value at the call sites. PR-URL: #61885 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
tmeijn
pushed a commit
to tmeijn/dotfiles
that referenced
this pull request
Mar 11, 2026
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [node](https://nodejs.org) ([source](https://github.com/nodejs/node)) | minor | `25.7.0` → `25.8.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>nodejs/node (node)</summary> ### [`v25.8.0`](https://github.com/nodejs/node/releases/tag/v25.8.0): 2026-03-03, Version 25.8.0 (Current), @​richardlau [Compare Source](nodejs/node@v25.7.0...v25.8.0) ##### Notable Changes - \[[`e55eddea2a`](nodejs/node@e55eddea2a)] - **build, doc**: use new api doc tooling (flakey5) [#​57343](nodejs/node#57343) - \[[`4c181e2277`](nodejs/node@4c181e2277)] - **(SEMVER-MINOR)** **sqlite**: add limits property to DatabaseSync (Mert Can Altin) [#​61298](nodejs/node#61298) - \[[`46ee1eddd7`](nodejs/node@46ee1eddd7)] - **(SEMVER-MINOR)** **src**: add C++ support for diagnostics channels (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`9ddd1a9c27`](nodejs/node@9ddd1a9c27)] - **(SEMVER-MINOR)** **src,permission**: add --permission-audit (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`0d97ec4044`](nodejs/node@0d97ec4044)] - **(SEMVER-MINOR)** **test\_runner**: expose worker ID for concurrent test execution (Ali Hassan) [#​61394](nodejs/node#61394) ##### Commits - \[[`940b58c8c1`](nodejs/node@940b58c8c1)] - **buffer**: optimize buffer.concat performance (Mert Can Altin) [#​61721](nodejs/node#61721) - \[[`0589b0e5a1`](nodejs/node@0589b0e5a1)] - **build**: fix GN for new merve dep (Shelley Vohr) [#​61984](nodejs/node#61984) - \[[`f3d3968dcd`](nodejs/node@f3d3968dcd)] - ***Revert*** "**build**: add temporal test on GHA windows" (Antoine du Hamel) [#​61810](nodejs/node#61810) - \[[`e55eddea2a`](nodejs/node@e55eddea2a)] - **build, doc**: use new api doc tooling (flakey5) [#​57343](nodejs/node#57343) - \[[`b7715292f8`](nodejs/node@b7715292f8)] - **child\_process**: add tracing channel for spawn (Marco) [#​61836](nodejs/node#61836) - \[[`a32a598748`](nodejs/node@a32a598748)] - **crypto**: fix missing nullptr check on RSA\_new() (ndossche) [#​61888](nodejs/node#61888) - \[[`dc384f95b3`](nodejs/node@dc384f95b3)] - **crypto**: fix handling of null BUF\_MEM\* in ToV8Value() (Nora Dossche) [#​61885](nodejs/node#61885) - \[[`3337b095db`](nodejs/node@3337b095db)] - **crypto**: fix potential null pointer dereference when BIO\_meth\_new() fails (Nora Dossche) [#​61788](nodejs/node#61788) - \[[`51ded81139`](nodejs/node@51ded81139)] - **deps**: update undici to 7.22.0 (Node.js GitHub Bot) [#​62035](nodejs/node#62035) - \[[`8aa2fde931`](nodejs/node@8aa2fde931)] - **deps**: update minimatch to 10.2.4 (Node.js GitHub Bot) [#​62016](nodejs/node#62016) - \[[`57dc092eaf`](nodejs/node@57dc092eaf)] - **deps**: upgrade npm to 11.11.0 (npm team) [#​61994](nodejs/node#61994) - \[[`705bbd60a9`](nodejs/node@705bbd60a9)] - **deps**: update simdjson to 4.3.1 (Node.js GitHub Bot) [#​61930](nodejs/node#61930) - \[[`4d411d72e5`](nodejs/node@4d411d72e5)] - **deps**: update acorn-walk to 8.3.5 (Node.js GitHub Bot) [#​61928](nodejs/node#61928) - \[[`f53a32ab84`](nodejs/node@f53a32ab84)] - **deps**: update acorn to 8.16.0 (Node.js GitHub Bot) [#​61925](nodejs/node#61925) - \[[`9b483fbb27`](nodejs/node@9b483fbb27)] - **deps**: update minimatch to 10.2.2 (Node.js GitHub Bot) [#​61830](nodejs/node#61830) - \[[`4e54c103cb`](nodejs/node@4e54c103cb)] - **doc**: separate in-types and out-types in SQLite conversion docs (René) [#​62034](nodejs/node#62034) - \[[`ca78ebbeaa`](nodejs/node@ca78ebbeaa)] - **doc**: fix small logic error in DETECT\_MODULE\_SYNTAX (René) [#​62025](nodejs/node#62025) - \[[`e6b131f3fe`](nodejs/node@e6b131f3fe)] - **doc**: fix module.stripTypeScriptTypes indentation (René) [#​61992](nodejs/node#61992) - \[[`7508540e19`](nodejs/node@7508540e19)] - **doc**: update DEP0040 (punycode) to application type deprecation (Mike McCready) [#​61916](nodejs/node#61916) - \[[`33a364cb62`](nodejs/node@33a364cb62)] - **doc**: explicitly mention Slack handle (Rafael Gonzaga) [#​61986](nodejs/node#61986) - \[[`46a61922bd`](nodejs/node@46a61922bd)] - **doc**: support toolchain Visual Studio 2022 & 2026 + Windows 11 SDK (Mike McCready) [#​61864](nodejs/node#61864) - \[[`dc12a257aa`](nodejs/node@dc12a257aa)] - **doc**: rename invalid `function` parameter (René) [#​61942](nodejs/node#61942) - \[[`dafdc0a5b8`](nodejs/node@dafdc0a5b8)] - **http**: validate headers in writeEarlyHints (Richard Clarke) [#​61897](nodejs/node#61897) - \[[`3c94b56fa6`](nodejs/node@3c94b56fa6)] - **inspector**: unwrap internal/debugger/inspect imports (René) [#​61974](nodejs/node#61974) - \[[`8a24c17648`](nodejs/node@8a24c17648)] - **lib**: improve argument handling in Blob constructor (Ms2ger) [#​61980](nodejs/node#61980) - \[[`21d4baf256`](nodejs/node@21d4baf256)] - **meta**: bump github/codeql-action from 4.32.0 to 4.32.4 (dependabot\[bot]) [#​61911](nodejs/node#61911) - \[[`59a726a8e3`](nodejs/node@59a726a8e3)] - **meta**: bump step-security/harden-runner from 2.14.1 to 2.14.2 (dependabot\[bot]) [#​61909](nodejs/node#61909) - \[[`0072b7f991`](nodejs/node@0072b7f991)] - **meta**: bump actions/stale from 10.1.1 to 10.2.0 (dependabot\[bot]) [#​61908](nodejs/node#61908) - \[[`999bf22f47`](nodejs/node@999bf22f47)] - **repl**: keep reference count for `process.on('newListener')` (Anna Henningsen) [#​61895](nodejs/node#61895) - \[[`4c181e2277`](nodejs/node@4c181e2277)] - **(SEMVER-MINOR)** **sqlite**: add limits property to DatabaseSync (Mert Can Altin) [#​61298](nodejs/node#61298) - \[[`aee2a18257`](nodejs/node@aee2a18257)] - **src**: fix flags argument offset in JSUdpWrap (Weixie Cui) [#​61948](nodejs/node#61948) - \[[`46ee1eddd7`](nodejs/node@46ee1eddd7)] - **(SEMVER-MINOR)** **src**: add C++ support for diagnostics channels (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`9ddd1a9c27`](nodejs/node@9ddd1a9c27)] - **(SEMVER-MINOR)** **src,permission**: add --permission-audit (RafaelGSS) [#​61869](nodejs/node#61869) - \[[`ea2df2a16f`](nodejs/node@ea2df2a16f)] - **stream**: fix pipeTo to defer writes per WHATWG spec (Matteo Collina) [#​61800](nodejs/node#61800) - \[[`aa0c7b09e0`](nodejs/node@aa0c7b09e0)] - **test**: remove unnecessary `process.exit` calls from test files (Antoine du Hamel) [#​62020](nodejs/node#62020) - \[[`ad96a6578f`](nodejs/node@ad96a6578f)] - **test**: skip `test-url` on `--shared-ada` builds (Antoine du Hamel) [#​62019](nodejs/node#62019) - \[[`7c72a31e4b`](nodejs/node@7c72a31e4b)] - **test**: skip strace test with shared openssl (Richard Lau) [#​61987](nodejs/node#61987) - \[[`604456c163`](nodejs/node@604456c163)] - **test**: avoid flaky debugger restart waits (Yuya Inoue) [#​61773](nodejs/node#61773) - \[[`4890d6bd43`](nodejs/node@4890d6bd43)] - **test\_runner**: run afterEach on runtime skip (Igor Shevelenkov) [#​61525](nodejs/node#61525) - \[[`fce2930110`](nodejs/node@fce2930110)] - **test\_runner**: expose expectFailure message (sangwook) [#​61563](nodejs/node#61563) - \[[`0d97ec4044`](nodejs/node@0d97ec4044)] - **(SEMVER-MINOR)** **test\_runner**: expose worker ID for concurrent test execution (Ali Hassan) [#​61394](nodejs/node#61394) - \[[`243e6b2009`](nodejs/node@243e6b2009)] - **test\_runner**: replace native methods with primordials (Ayoub Mabrouk) [#​61219](nodejs/node#61219) - \[[`bf1ed7e647`](nodejs/node@bf1ed7e647)] - **tls**: forward keepAlive, keepAliveInitialDelay, noDelay to socket (Sergey Zelenov) [#​62004](nodejs/node#62004) - \[[`0f15079d94`](nodejs/node@0f15079d94)] - **tools**: remove custom logic for skipping `test-strace-openat-openssl` (Antoine du Hamel) [#​62038](nodejs/node#62038) - \[[`54a055a59d`](nodejs/node@54a055a59d)] - **tools**: bump minimatch from 3.1.2 to 3.1.3 in `/tools/clang-format` (dependabot\[bot]) [#​61977](nodejs/node#61977) - \[[`a28744cb62`](nodejs/node@a28744cb62)] - **tools**: fix permissions for merve update script (Richard Lau) [#​62023](nodejs/node#62023) - \[[`31e7936354`](nodejs/node@31e7936354)] - **tools**: revert tools GHA workflow to ubuntu-latest (Richard Lau) [#​62024](nodejs/node#62024) - \[[`0a96a16e1f`](nodejs/node@0a96a16e1f)] - **tools**: bump minimatch from 3.1.2 to 3.1.3 in /tools/eslint (dependabot\[bot]) [#​61976](nodejs/node#61976) - \[[`f279233412`](nodejs/node@f279233412)] - **tools**: roll back to x86 runner on `scorecard.yml` (Antoine du Hamel) [#​61944](nodejs/node#61944) - \[[`192c0382f4`](nodejs/node@192c0382f4)] - **util**: add fast path to stripVTControlCharacters (Hiroki Osame) [#​61833](nodejs/node#61833) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My40OS4wIiwidXBkYXRlZEluVmVyIjoiNDMuNDkuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The assignment to
bptrcallsBIO_get_mem_ptrwhich can fail and leave thebptras nullptr. This then later causes a null pointer deref.This is inconsistent with uses of the similar function
BIO_get_mem_datathat do check its return value, e.g.node::crypto::X509sToArrayOfStrings().Solve it by checking for a null pointer and handling the
Nothingreturn value at the call sites.Note: this was found by a static-dynamic analyser I'm developing.